Skip to content

refactor: extract named backups list facade#294

Closed
ndycode wants to merge 1 commit intorefactor/pr3-storage-save-flagged-entryfrom
refactor/pr3-storage-named-backups-list-entry
Closed

refactor: extract named backups list facade#294
ndycode wants to merge 1 commit intorefactor/pr3-storage-save-flagged-entryfrom
refactor/pr3-storage-named-backups-list-entry

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • extract the getNamedBackups facade out of lib/storage.ts on top of the latest storage leaf
  • keep named backup listing behavior unchanged while slimming the storage facade
  • add a focused test for the new named backups entry helper

Validation

  • npm run typecheck
  • npm run lint -- lib/storage/named-backups-entry.ts lib/storage.ts test/named-backups-entry.test.ts
  • npm run test -- test/named-backups-entry.test.ts

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr extracts the getNamedBackups facade in lib/storage.ts into a dedicated lib/storage/named-backups-entry.ts module, following the same dep-injection pattern already used by restoreAccountsFromBackupEntry. behaviour is unchanged; the extraction improves testability and keeps storage.ts slimmer.

  • lib/storage/named-backups-entry.ts — new thin entry that calls collectNamedBackups via fully injected deps (getStoragePath, collectNamedBackups, loadAccountsFromPath, logDebug)
  • lib/storage.tsgetNamedBackups is now a one-liner delegating to getNamedBackupsEntry; dep wiring is correct
  • test/named-backups-entry.test.ts — focused vitest case asserting all four deps are wired correctly and the return value propagates; adequate coverage for this pass-through layer
  • one style nit: named-backups-entry.ts imports NamedBackupSummary from ../storage.js (the parent facade) rather than from ./named-backups.js where the interface is actually defined — recommend fixing to break the apparent type-level cycle

Confidence Score: 5/5

  • safe to merge — no behavioural change, correct dep wiring, one cosmetic import nit
  • the refactor is a pure structural extraction with no logic change. dep injection pattern matches the existing restoreAccountsFromBackupEntry precedent. the single test is sufficient for a pass-through facade. the only flag is the import type sourced from the parent facade rather than the sibling definition — a p2 style issue that doesn't affect correctness or runtime behaviour.
  • lib/storage/named-backups-entry.ts — fix the NamedBackupSummary import source

Important Files Changed

Filename Overview
lib/storage/named-backups-entry.ts new thin facade that delegates to collectNamedBackups via injected deps — correct behaviour, but imports NamedBackupSummary from ../storage.js instead of the sibling ./named-backups.js where it's defined, creating an apparent type-level circular reference
lib/storage.ts minimal change — adds import for getNamedBackupsEntry and rewires getNamedBackups to use it; wiring matches the same pattern as restoreAccountsFromBackupEntry, all deps passed correctly
test/named-backups-entry.test.ts focused unit test, mocks all four deps, asserts correct argument wiring and return value propagation — adequate for this thin layer

Sequence Diagram

sequenceDiagram
    participant caller as caller
    participant storage as lib/storage.ts<br/>getNamedBackups()
    participant entry as lib/storage/named-backups-entry.ts<br/>getNamedBackupsEntry()
    participant nb as lib/storage/named-backups.ts<br/>collectNamedBackups()

    caller->>storage: getNamedBackups()
    storage->>entry: getNamedBackupsEntry({ getStoragePath, collectNamedBackups, loadAccountsFromPath, logDebug })
    entry->>entry: getStoragePath() → storagePath
    entry->>nb: collectNamedBackups(storagePath, { loadAccountsFromPath, logDebug })
    nb-->>entry: NamedBackupSummary[]
    entry-->>storage: NamedBackupSummary[]
    storage-->>caller: NamedBackupSummary[]
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/storage/named-backups-entry.ts
Line: 1

Comment:
**import `NamedBackupSummary` from its origin, not the parent facade**

`NamedBackupSummary` is defined in `./named-backups.ts` (right next to this file). importing it through `../storage.js` creates an apparent type-level cycle: `storage.ts` imports `getNamedBackupsEntry` from here, and this file imports back from `storage.ts`. it's safe at runtime because it's `import type`, but it's misleading and tighter to source the type where it lives.

```suggestion
import type { NamedBackupSummary } from "./named-backups.js";
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: extract na..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 15 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e3eba252-a701-4589-8b95-81cb591d101c

📥 Commits

Reviewing files that changed from the base of the PR and between 6b594b9 and c64566f.

📒 Files selected for processing (3)
  • lib/storage.ts
  • lib/storage/named-backups-entry.ts
  • test/named-backups-entry.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr3-storage-named-backups-list-entry
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr3-storage-named-backups-list-entry

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant